-
Notifications
You must be signed in to change notification settings - Fork 301
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Exam Mode
: Add sidebar to student view to display exams
#8662
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
Outside diff range comments (3)
src/test/javascript/spec/component/shared/sidebar/sidebar-card-large.component.spec.ts (1)
Line range hint
24-34
: Avoid duplicatingbeforeEach
setup hooks within the samedescribe
block to ensure clarity and efficiency in test setup.- beforeEach(() => { - router = new MockRouter(); - TestBed.configureTestingModule({ - imports: [ArtemisTestModule, MockModule(RouterModule)], - declarations: [SidebarCardLargeComponent, SidebarCardItemComponent, MockRouterLinkDirective], - providers: [{ provide: Router, useValue: router }], - }).compileComponents(); - });src/test/javascript/spec/component/shared/sidebar/sidebar.component.spec.ts (1)
Line range hint
39-44
: DuplicatebeforeEach
setup hooks are present, which can lead to confusion and redundancy in test configurations.- beforeEach(() => { - TestBed.configureTestingModule({ - imports: [ArtemisTestModule, MockModule(FormsModule), MockModule(ReactiveFormsModule), MockModule(RouterModule)], - declarations: [ - SidebarComponent, - SidebarCardMediumComponent, - SidebarCardItemComponent, - SidebarCardDirective, - SearchFilterPipe, - SearchFilterComponent, - MockPipe(ArtemisTranslatePipe), - MockRouterLinkDirective, - ], - }).compileComponents(); - });src/main/webapp/app/exercises/quiz/participate/quiz-participation.component.html (1)
[!TIP]
Codebase VerificationThe
QuizMode
enum definitions in both files include the modes:SYNCHRONIZED
,BATCHED
, andINDIVIDUAL
. However, the switch-case block in the provided HTML snippet only handlespractice
andpreview
modes. This discrepancy indicates that not all possible quiz modes are covered in the switch-case block.
SYNCHRONIZED
BATCHED
INDIVIDUAL
Ensure all quiz modes are handled in the switch-case block to prevent any unhandled cases.
Analysis chain
Line range hint
7-13
: Ensure that all possible quiz modes are handled in the switch-case block to prevent any unhandled cases.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all quiz modes are handled in the switch-case block. # Test: Search for all quiz mode definitions and ensure they are covered in the switch-case. rg --type typescript 'enum QuizMode'Length of output: 73
Script:
#!/bin/bash # Description: Verify all quiz modes are handled in the switch-case block. # Test: Search for all quiz mode definitions and ensure they are covered in the switch-case. rg --type ts 'enum QuizMode'Length of output: 174
Script:
#!/bin/bash # Description: Extract QuizMode enum definitions from the identified files. # Extracting QuizMode enum from src/test/playwright/support/constants.ts rg --type ts 'export enum QuizMode' -A 5 src/test/playwright/support/constants.ts # Extracting QuizMode enum from src/main/webapp/app/entities/quiz/quiz-exercise.model.ts rg --type ts 'export enum QuizMode' -A 5 src/main/webapp/app/entities/quiz/quiz-exercise.model.tsLength of output: 417
src/main/webapp/app/exam/exam-scores/exam-scores.component.html
Outdated
Show resolved
Hide resolved
src/test/javascript/spec/component/shared/sidebar/sidebar-card-large.component.spec.ts
Show resolved
Hide resolved
src/main/webapp/app/exam/participate/exam-cover/exam-participation-cover.component.html
Outdated
Show resolved
Hide resolved
49bddb3
to
b5cce62
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
Outside diff range comments (5)
src/test/javascript/spec/component/shared/sidebar/sidebar-card-large.component.spec.ts (1)
Line range hint
24-34
: Remove duplicatebeforeEach
block to adhere to best practices and avoid unnecessary test setup duplication.- beforeEach(() => { - router = new MockRouter(); - TestBed.configureTestingModule({ - imports: [ArtemisTestModule, MockModule(RouterModule)], - declarations: [SidebarCardLargeComponent, SidebarCardItemComponent, MockRouterLinkDirective], - providers: [{ provide: Router, useValue: router }], - }).compileComponents(); - });src/test/javascript/spec/component/shared/sidebar/sidebar.component.spec.ts (1)
Line range hint
39-44
: ThebeforeEach
block is duplicated, which could lead to maintenance issues and unexpected behavior in tests. Consider removing the duplicate to streamline the test setup.- beforeEach(() => { - TestBed.configureTestingModule({ - imports: [ArtemisTestModule, MockModule(FormsModule), MockModule(ReactiveFormsModule), MockModule(RouterModule)], - declarations: [ - SidebarComponent, - SidebarCardMediumComponent, - SidebarCardItemComponent, - SidebarCardDirective, - SearchFilterPipe, - SearchFilterComponent, - MockPipe(ArtemisTranslatePipe), - MockRouterLinkDirective, - ], - }).compileComponents(); - });src/main/webapp/app/overview/course-statistics/course-statistics.component.html (1)
[!TIP]
Codebase VerificationThe search results indicate that
@if
and@for
directives are used extensively in the codebase. However, these directives are not standard Angular directives, which suggests a potential issue.
- Location to address:
- Replace
@if
with*ngIf
- Replace
@for
with*ngFor
Ensure that the correct Angular directives are used for conditional rendering and iteration.
Analysis chain
Line range hint
1-1
: Review the usage of@if
and@for
directives.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of `@if` and `@for` to ensure they are correctly used in place of *ngIf and *ngFor. # Test: Search for the directives in Angular HTML templates. Expect: Only valid usages of `@if` and `@for`. rg --type html '@if|@for' src/main/webapp/**/*.htmlLength of output: 121
Script:
#!/bin/bash # Description: Correct the file path issue and search for usages of `@if` and `@for` directives in Angular HTML templates. # Locate all HTML files within src/main/webapp/app/overview/course-statistics/ fd --type f --extension html --exec rg '@if|@for' {}Length of output: 269708
src/main/webapp/app/exercises/shared/dashboards/tutor/exercise-assessment-dashboard.component.html (2)
Line range hint
3-5
: Ensure proper null checking forexercise
when accessing properties liketitle
to prevent runtime errors.Consider using optional chaining (
exercise?.title
) to safely access properties.
Line range hint
33-33
: The repeated conditional check fornumberOfSubmissions.late > 0
could be optimized by storing the result in a temporary variable and using it in the template to reduce redundancy.- @if (numberOfSubmissions.late > 0) { + let hasLateSubmissions = numberOfSubmissions.late > 0; + @if (hasLateSubmissions) {
88f0784
to
b5cce62
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
Outside diff range comments (1)
src/test/javascript/spec/component/shared/sidebar/sidebar-card-large.component.spec.ts (1)
Line range hint
24-34
: Refactor to combine duplicate setup hooks.There are two
beforeEach
blocks that can be combined to streamline the setup process and reduce redundancy.- beforeEach(() => { - router = new MockRouter(); - TestBed.configureTestingModule({ - imports: [ArtemisTestModule, MockModule(RouterModule)], - declarations: [SidebarCardLargeComponent, SidebarCardItemComponent, MockRouterLinkDirective], - providers: [{ provide: Router, useValue: router }], - }).compileComponents(); -}); - - beforeEach(() => { - fixture = TestBed.createComponent(SidebarCardLargeComponent); - component = fixture.componentInstance; - component.sidebarItem = { - title: 'testTitle', - id: 'testId', - size: 'L', - }; - component.itemSelected = true; - fixture.detectChanges(); -}); + beforeEach(() => { + router = new MockRouter(); + TestBed.configureTestingModule({ + imports: [ArtemisTestModule, MockModule(RouterModule)], + declarations: [SidebarCardLargeComponent, SidebarCardItemComponent, MockRouterLinkDirective], + providers: [{ provide: Router, useValue: router }], + }).compileComponents(); + + fixture = TestBed.createComponent(SidebarCardLargeComponent); + component = fixture.componentInstance; + component.sidebarItem = { + title: 'testTitle', + id: 'testId', + size: 'L', + }; + component.itemSelected = true; + fixture.detectChanges(); +});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range comments (1)
src/main/webapp/app/types/sidebar.ts (1)
Line range hint
56-56
: Avoid Usingany
for Type Definitions:The use of
any
forquickActionIcons
should be avoided as it bypasses TypeScript's type checking. It's recommended to define a more specific type that accurately describes the expected properties.- quickActionIcons?: any; + quickActionIcons?: QuickActionIcon[]; // Define this type based on the expected properties
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reapprove after merge conflict
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reapprove after merge
Checklist
General
Client
authorities
to all new routes and checked the course groups for displaying navigation elements (links, buttons).Motivation and Context
In the current design of Artemis, we display real and test exams side by side. In other parts of Artemis, such as lectures and exercises, we use sidebars to group elements. To maintain consistency, the sidebar is also implemented in the course exams view in order to group and display real and test exams. This implementation is therefore a part of the new design in exam mode.
Description
Steps for Testing
Prerequisites:
(There are already three exams available under my course "Ege Dogu Kaya" on TS4 and only one test account (artemis_test_user_4) is registered for these three exams, feel free to register other accounts if needed. If you want to use these three exams to test it, you can skip first 8 steps. )
Exam Mode Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Review Progress
Performance Review
Code Review
Manual Tests
Exam Mode Test
Test Coverage
Screenshots
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Documentation